fix: switch hide system message not working on mobile and loading on older server#7130
fix: switch hide system message not working on mobile and loading on older server#7130OtavioStasiak wants to merge 19 commits intodevelopfrom
Conversation
OtavioStasiak
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe changes introduce a room type parameter ( Changes
Sequence DiagramsequenceDiagram
participant Component as ListContainer Component
participant Hook as useMessages Hook
participant Query as buildVisibleSystemTypesClause
participant DB as WatermelonDB
participant Redux as Redux Store
participant Server as Server API
Component->>Hook: Call with rid, t, hideSystemMessages, serverVersion
alt Initial / rid changed
Hook->>Query: buildVisibleSystemTypesClause(hideSystemMessages)
Query-->>Hook: Q.Or condition or null
Hook->>DB: Query messages with visibility condition
DB-->>Hook: Observable emitting filtered rows
Hook->>Hook: Store in rawMessages state
Hook->>Hook: useMemo computes visibleMessages from rawMessages
Hook->>Component: Return visibleMessages
end
alt serverVersion >= 3.16 && hideSystemMessages active
Hook->>Hook: Search visibleMessages for loader message
alt Loader message found
Hook->>Redux: Dispatch roomHistoryRequest({rid, t, loaderId})
Redux->>Server: Send history request
Server-->>Redux: Update history state
end
end
Component->>Component: Render messages from hook result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
OtavioStasiak
temporarily deployed
to
approve_e2e_testing
GitHub Actions
Inactive
OtavioStasiak
changed the title
OtavioStasiak
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/views/RoomView/List/hooks/useMessages.test.tsx`:
- Around line 55-60: The helper msg function currently sets id twice causing an
ESLint duplicate-property warning; change the object literal to avoid
duplicating id by spreading overrides only once (e.g., put ts default first and
then ...overrides), so update the msg function (symbol: msg, parameter:
overrides, type: TAnyMessageModel) to return { ts: new Date('2024-01-01'),
...overrides } as TAnyMessageModel (or alternatively remove the explicit id:
overrides.id) to eliminate the duplicate property.
- Around line 70-79: The mock for mockDbGet currently returns an object that
doesn't satisfy the expected Collection<Model> type; update the mock
implementation in useMessages.test.tsx so the returned value is asserted as the
proper type (e.g., use "as unknown as Collection<Model>" or similar) while
keeping the existing structure: the mockDbGet.mockImplementation that returns {
query: jest.fn().mockReturnValue({ observe: () => ({ subscribe: (onNext: (rows:
TAnyMessageModel[]) => void) => { onNext(emittedRows); return { unsubscribe:
jest.fn() }; } }) }) } should be wrapped with the TypeScript type assertion to
Collection<Model> so the compiler accepts the mock.
In `@app/views/RoomView/List/hooks/useMessages.ts`:
- Around line 136-143: The useEffect block using compareServerVersion,
visibleMessages and hideSystemMessages should include all external dependencies
to avoid stale closures: add serverVersion, rid, t and dispatch to the
dependency array (in addition to hideSystemMessages and visibleMessages) and
ensure the loader lookup uses visibleMessages and MESSAGE_TYPE_ANY_LOAD as
before; also change the conditional from "hideSystemMessages && loaderId" to
"hideSystemMessages.length > 0 && loaderId" so an empty array doesn't
incorrectly pass; keep roomHistoryRequest(dispatch...) invocation with the found
loaderId.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: dfe34416-508f-4f88-a99c-22e6a326485a
📒 Files selected for processing (7)
app/views/RoomView/List/definitions.tsapp/views/RoomView/List/hooks/buildVisibleSystemTypesClause.tsapp/views/RoomView/List/hooks/useMessages.test.tsxapp/views/RoomView/List/hooks/useMessages.tsapp/views/RoomView/List/hooks/useScroll.tsapp/views/RoomView/List/index.tsxapp/views/RoomView/index.tsx
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: E2E Build Android / android-build
- GitHub Check: E2E Build iOS / ios-build
- GitHub Check: format
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{js,jsx,ts,tsx,json}
📄 CodeRabbit inference engine (CLAUDE.md)
Configure Prettier with tabs, single quotes, 130 character width, no trailing commas, arrow parens avoid, and bracket same line
Files:
app/views/RoomView/List/index.tsxapp/views/RoomView/index.tsxapp/views/RoomView/List/definitions.tsapp/views/RoomView/List/hooks/buildVisibleSystemTypesClause.tsapp/views/RoomView/List/hooks/useScroll.tsapp/views/RoomView/List/hooks/useMessages.tsapp/views/RoomView/List/hooks/useMessages.test.tsx
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use ESLint with
@rocket.chat/eslint-configbase configuration including React, React Native, TypeScript, and Jest plugins
Files:
app/views/RoomView/List/index.tsxapp/views/RoomView/index.tsxapp/views/RoomView/List/definitions.tsapp/views/RoomView/List/hooks/buildVisibleSystemTypesClause.tsapp/views/RoomView/List/hooks/useScroll.tsapp/views/RoomView/List/hooks/useMessages.tsapp/views/RoomView/List/hooks/useMessages.test.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use TypeScript with strict mode enabled and configure baseUrl to app/ for import resolution
**/*.{ts,tsx}: Use TypeScript for type safety; add explicit type annotations to function parameters and return types
Prefer interfaces over type aliases for defining object shapes in TypeScript
Use enums for sets of related constants rather than magic strings or numbers
Files:
app/views/RoomView/List/index.tsxapp/views/RoomView/index.tsxapp/views/RoomView/List/definitions.tsapp/views/RoomView/List/hooks/buildVisibleSystemTypesClause.tsapp/views/RoomView/List/hooks/useScroll.tsapp/views/RoomView/List/hooks/useMessages.tsapp/views/RoomView/List/hooks/useMessages.test.tsx
app/views/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Place screen components in app/views/ directory
Files:
app/views/RoomView/List/index.tsxapp/views/RoomView/index.tsxapp/views/RoomView/List/definitions.tsapp/views/RoomView/List/hooks/buildVisibleSystemTypesClause.tsapp/views/RoomView/List/hooks/useScroll.tsapp/views/RoomView/List/hooks/useMessages.tsapp/views/RoomView/List/hooks/useMessages.test.tsx
**/*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,ts,jsx,tsx}: Use descriptive names for functions, variables, and classes that clearly convey their purpose
Write comments that explain the 'why' behind code decisions, not the 'what'
Keep functions small and focused on a single responsibility
Use const by default, let when reassignment is needed, and avoid var
Prefer async/await over .then() chains for handling asynchronous operations
Use explicit error handling with try/catch blocks for async operations
Avoid deeply nested code; refactor complex logic into helper functions
Files:
app/views/RoomView/List/index.tsxapp/views/RoomView/index.tsxapp/views/RoomView/List/definitions.tsapp/views/RoomView/List/hooks/buildVisibleSystemTypesClause.tsapp/views/RoomView/List/hooks/useScroll.tsapp/views/RoomView/List/hooks/useMessages.tsapp/views/RoomView/List/hooks/useMessages.test.tsx
🧠 Learnings (2)
📚 Learning: 2026-04-07T17:49:17.538Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat.ReactNative PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-07T17:49:17.538Z
Learning: Applies to app/definitions/**/*.{ts,tsx} : Place shared TypeScript type definitions in app/definitions/
Applied to files:
app/views/RoomView/List/definitions.ts
📚 Learning: 2026-04-07T17:49:17.538Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat.ReactNative PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-07T17:49:17.538Z
Learning: Applies to app/lib/database/model/**/*.{ts,tsx} : Place database models in app/lib/database/model/ for entities like Message, Room, Subscription, User, Thread, Upload, Server, CustomEmoji, Permission, and Role
Applied to files:
app/views/RoomView/List/hooks/useMessages.ts
🪛 GitHub Check: ESLint and Test / run-eslint-and-test
app/views/RoomView/List/hooks/useMessages.test.tsx
[failure] 70-70:
Argument of type '() => { query: jest.Mock<any, any, any>; }' is not assignable to parameter of type '(tableName: string) => Collection'.
[failure] 57-57:
'id' is specified more than once, so this usage will be overwritten.
🔇 Additional comments (8)
app/views/RoomView/List/hooks/buildVisibleSystemTypesClause.ts (1)
10-17: LGTM - Clean implementation of the visibility clause builder.The logic correctly implements the filter: messages pass if
tis null (regular messages),tis a load-type marker, ortis not in the hidden list. TheQ.and(...map(Q.notEq))pattern correctly implements "not in list" semantics.app/views/RoomView/index.tsx (1)
1674-1674: LGTM - Correctly passes room type to List component.The prop addition aligns with the updated
IListContainerPropsinterface requirements.app/views/RoomView/List/index.tsx (1)
10-18: LGTM - Props and hook invocation correctly updated.The
tprop is properly destructured and passed touseMessagesalong with the reordered arguments matching the updated hook signature.app/views/RoomView/List/definitions.ts (1)
5-5: LGTM - Interface properly extended with room type.The
RoomTypeimport from shared definitions and the new requiredtproperty follow existing patterns. Based on learnings, shared TypeScript type definitions are correctly sourced fromapp/definitions/.Also applies to: 24-24
app/views/RoomView/List/hooks/useScroll.ts (1)
7-17: LGTM - Correctly handles stale closure prevention.The pattern of accepting
messagesIds: string[]as a prop and synchronizing it to a ref viauseEffectcorrectly prevents stale closures in the asyncjumpToMessagecallback. This is a proper React pattern for maintaining up-to-date values in callbacks without causing re-renders.app/views/RoomView/List/hooks/useMessages.test.tsx (1)
127-144: Good test coverage for the version-gated dispatch behavior.The test correctly verifies that
ROOM.HISTORY_REQUESTis dispatched when server version is>= 3.16.0, system messages are hidden, and a load row exists. This covers the core fix for the PR objective.app/views/RoomView/List/hooks/useMessages.ts (2)
124-129: LGTM - Efficient memoized filtering.The
useMemocorrectly derives visible messages from raw messages, filtering out hidden system message types. The early return whenhideSystemMessagesis empty avoids unnecessary filtering.
48-86: LGTM - DB-level filtering ensures correct pagination.Applying
buildVisibleSystemTypesClauseat the query level ensuresQ.take(count.current)retrieves the expected number of visible rows, fixing the core pagination issue when many system messages are hidden.
OtavioStasiak
temporarily deployed
to
approve_e2e_testing
GitHub Actions
Inactive
OtavioStasiak
had a problem deploying
to
experimental_android_build
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
official_ios_build
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
experimental_ios_build
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
official_android_build
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
official_ios_build
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
official_android_build
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
experimental_ios_build
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
experimental_android_build
GitHub Actions
Error
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
✅ Actions performedFull review triggered. |
OtavioStasiak
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
OtavioStasiak
temporarily deployed
to
approve_e2e_testing
GitHub Actions
Inactive
OtavioStasiak
had a problem deploying
to
official_android_build
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
experimental_android_build
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
experimental_ios_build
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
official_ios_build
GitHub Actions
Error
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/views/RoomView/List/hooks/useMessages.ts (1)
131-131: Optional: avoid mutating a ref during render.
messagesIds.current = visibleMessages.map(m => m.id);is executed in the render body on every render, including renders unrelated to message changes. React's docs advise against writing to refs during rendering, and this also runs.map()unnecessarily whenvisibleMessagesis a stable memoized reference.Consider folding it into the
useMemo(or a smalluseEffect) so it only updates whenvisibleMessagesactually changes:♻️ Proposed refactor
- const visibleMessages = useMemo(() => { - if (!hideSystemMessages || hideSystemMessages.length === 0) { - return rawMessages; - } - return rawMessages.filter(m => !m.t || !hideSystemMessages.includes(m.t)); - }, [rawMessages, hideSystemMessages]); - - messagesIds.current = visibleMessages.map(m => m.id); + const visibleMessages = useMemo(() => { + if (!hideSystemMessages || hideSystemMessages.length === 0) { + return rawMessages; + } + return rawMessages.filter(m => !m.t || !hideSystemMessages.includes(m.t)); + }, [rawMessages, hideSystemMessages]); + + useEffect(() => { + messagesIds.current = visibleMessages.map(m => m.id); + }, [visibleMessages]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/views/RoomView/List/hooks/useMessages.ts` at line 131, The assignment messagesIds.current = visibleMessages.map(m => m.id) mutates a ref during render; move this work into a hook that runs only when visibleMessages changes (e.g., compute ids with useMemo(() => visibleMessages.map(m => m.id), [visibleMessages]) and assign the result to messagesIds.current inside a useEffect or assign the memoized value directly in the same useMemo callback), so messagesIds is updated only when visibleMessages changes and you avoid unnecessary .map() on every render; update the code around messagesIds and visibleMessages in useMessages.ts accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@app/views/RoomView/List/hooks/useMessages.ts`:
- Line 131: The assignment messagesIds.current = visibleMessages.map(m => m.id)
mutates a ref during render; move this work into a hook that runs only when
visibleMessages changes (e.g., compute ids with useMemo(() =>
visibleMessages.map(m => m.id), [visibleMessages]) and assign the result to
messagesIds.current inside a useEffect or assign the memoized value directly in
the same useMemo callback), so messagesIds is updated only when visibleMessages
changes and you avoid unnecessary .map() on every render; update the code around
messagesIds and visibleMessages in useMessages.ts accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 134a2dfa-9ff2-4844-b38d-4a4f6e696f33
📒 Files selected for processing (2)
app/views/RoomView/List/hooks/useMessages.test.tsxapp/views/RoomView/List/hooks/useMessages.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- app/views/RoomView/List/hooks/useMessages.test.tsx
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: E2E Build iOS / ios-build
- GitHub Check: E2E Build Android / android-build
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{js,jsx,ts,tsx,json}
📄 CodeRabbit inference engine (CLAUDE.md)
Configure Prettier with tabs, single quotes, 130 character width, no trailing commas, arrow parens avoid, and bracket same line
Files:
app/views/RoomView/List/hooks/useMessages.ts
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use ESLint with
@rocket.chat/eslint-configbase configuration including React, React Native, TypeScript, and Jest plugins
Files:
app/views/RoomView/List/hooks/useMessages.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use TypeScript with strict mode enabled and configure baseUrl to app/ for import resolution
**/*.{ts,tsx}: Use TypeScript for type safety; add explicit type annotations to function parameters and return types
Prefer interfaces over type aliases for defining object shapes in TypeScript
Use enums for sets of related constants rather than magic strings or numbers
Files:
app/views/RoomView/List/hooks/useMessages.ts
app/views/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Place screen components in app/views/ directory
Files:
app/views/RoomView/List/hooks/useMessages.ts
**/*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,ts,jsx,tsx}: Use descriptive names for functions, variables, and classes that clearly convey their purpose
Write comments that explain the 'why' behind code decisions, not the 'what'
Keep functions small and focused on a single responsibility
Use const by default, let when reassignment is needed, and avoid var
Prefer async/await over .then() chains for handling asynchronous operations
Use explicit error handling with try/catch blocks for async operations
Avoid deeply nested code; refactor complex logic into helper functions
Files:
app/views/RoomView/List/hooks/useMessages.ts
🧠 Learnings (1)
📚 Learning: 2026-04-07T17:49:17.538Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat.ReactNative PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-07T17:49:17.538Z
Learning: Applies to app/lib/database/model/**/*.{ts,tsx} : Place database models in app/lib/database/model/ for entities like Message, Room, Subscription, User, Thread, Upload, Server, CustomEmoji, Permission, and Role
Applied to files:
app/views/RoomView/List/hooks/useMessages.ts
🔇 Additional comments (1)
app/views/RoomView/List/hooks/useMessages.ts (1)
124-144: LGTM — prior review comments addressed.Both concerns from the previous review are now resolved:
- The effect's dependency array (line 144) correctly includes
serverVersion,rid,t,hideSystemMessages,visibleMessages, anddispatch.- Line 140 now uses
hideSystemMessages.length && loaderId, avoiding the empty-array truthiness bug.The DB-level filter via
buildVisibleSystemTypesClauseinfetchMessages(withhideSystemMessagesin its deps at line 100) ensuresQ.take(count.current)returns enough visible rows for pagination on servers below 3.16, and the new effect handles the 3.16+ case where the backend omits messages. TheuseMemoforvisibleMessagesis also valid because the subscription callback at line 91 spreads into a new array, givingrawMessagesa fresh reference on each emit.
…r concurrent rendering
OtavioStasiak
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
official_android_build
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
official_ios_build
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
experimental_android_build
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
experimental_ios_build
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
official_android_build
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
experimental_ios_build
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
official_ios_build
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
experimental_android_build
GitHub Actions
Error
OtavioStasiak
requested a deployment
to
approve_e2e_testing
GitHub Actions
Waiting
OtavioStasiak
requested a deployment
to
experimental_ios_build
GitHub Actions
Waiting
OtavioStasiak
requested a deployment
to
official_ios_build
GitHub Actions
Waiting
OtavioStasiak
requested a deployment
to
experimental_android_build
GitHub Actions
Waiting
OtavioStasiak
requested a deployment
to
official_android_build
GitHub Actions
Waiting
2 participants
Proposed changes
Fix an issue where the ‘hide system messages’ toggle doesn’t apply on mobile, and improve pagination for servers with more than 50 system messages (especially older ones).
Issue(s)
https://rocketchat.atlassian.net/browse/SUP-1021
How to test or reproduce
Case I
Case II
Case III
Case IV
Screenshots
Types of changes
Checklist
Further comments
Summary by CodeRabbit
New Features
Tests